-
Notifications
You must be signed in to change notification settings - Fork 594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GnarlyGenotyper tech debt #6075
Conversation
@ldgauthier Could you please rebase this branch onto the latest master, so that we can see whether tests pass? |
AnnotationUtils.isAlleleSpecific()"
--floor-blocks HaplotypeCaller arg" Also clarified beta status for GVCF mode just for Mutect2
GenomicsDB options
7349e92
to
2c277d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Back to @ldgauthier with my comments
return true; | ||
} | ||
if (annotation instanceof AS_StandardAnnotation) { | ||
if (annotation instanceof AlleleSpecificAnnotation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch references the new AlleleSpecificAnnotation
interface in several places, but does not seem to actually introduce it. Maybe you just forgot to git add
it?
if (hcArgs.floorBlocks && !emitReferenceConfidence()) { | ||
throw new UserException(HaplotypeCallerArgumentCollection.OUTPUT_BLOCK_LOWER_BOUNDS + " refers to GVCF blocks," + | ||
" so reference confidence mode (" + AssemblyBasedCallerArgumentCollection.EMIT_REFERENCE_CONFIDENCE_LONG_NAME + | ||
") must be specified."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You included this check twice here.
@@ -371,6 +372,43 @@ else if (!vc.getAlternateAllele(0).equals(Allele.NON_REF_ALLELE)){ //there | |||
} | |||
} | |||
|
|||
/* | |||
* Test that in VCF mode we're consistent with past GATK4 results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment does not match the test. Should be something like "Test the --floor-blocks
argument."
|
||
final ArgumentsBuilder args = new ArgumentsBuilder().addInput(new File(inputFileName)) | ||
.addReference(new File(referenceFileName)) | ||
.addInterval(new SimpleInterval("20:10000000-10100000")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This interval might be unnecessarily large for this test -- could you shrink it down?
return true; | ||
} | ||
if (annotation instanceof AS_StandardAnnotation) { | ||
if (annotation instanceof AlleleSpecificAnnotation) { | ||
return true; | ||
} | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return annotation instanceof AlleleSpecificAnnotation;
would be more compact
* This is similar to the HaplotypeCaller reference confidence/GVCF mode. See https://software.broadinstitute.org/gatk/documentation/article.php?id=4017 for information about GVCFs. | ||
* The reference confidence mode makes it possible to emit a per-bp or summarized confidence estimate for a site being strictly homozygous-reference. | ||
* See https://software.broadinstitute.org/gatk/documentation/article.php?id=4017 for information about GVCFs. | ||
* This is a BETA FEATURE for Mutect2 similar to the HaplotypeCaller reference confidence/GVCF mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend rewording slightly to put "Mutect2" at the start of the sentence before the word "BETA": "For Mutect2, this is a BETA feature that functions similarly to the HaplotypeCaller reference confidence/GVCF mode."
*/ | ||
@Advanced | ||
@Argument(fullName="emit-ref-confidence", shortName="ERC", doc="(BETA feature) Mode for emitting reference confidence scores", optional = true) | ||
@Argument(fullName=EMIT_REFERENCE_CONFIDENCE_LONG_NAME, shortName="ERC", doc="Mode for emitting reference confidence scores (BETA for Mutect2)", optional = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(BETA for Mutect2)
-> (For Mutect2, this is a BETA feature)
Sorry, that was sloppy. I expect that now it builds. Addressed all the review comments. |
Looks like travis failed with an out-of-memory error (exit code 137) in gradle. @lbergelson was investigating this error earlier, and we thought we'd fixed it... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 This looks good now -- we can merge as soon as travis turns green
Addressing @droazen 's outstanding comments from #4947
Also closes #5988